C++: include stack-allocated arrays in off-by-one query#13116
C++: include stack-allocated arrays in off-by-one query#13116rdmarsh2 merged 7 commits intogithub:mainfrom
Conversation
| module ArrayAddressToDerefConfig implements DataFlow::StateConfigSig { | ||
| newtype FlowState = | ||
| additional TArray(Field f) or | ||
| additional TArray(Variable v) or |
There was a problem hiding this comment.
As I mentioned in #13045, we can probably restrict these variables to be the ones satisfying pointerArithOverflow(_, v, _, _, _).
a96cc6d to
6e230e1
Compare
MathiasVP
left a comment
There was a problem hiding this comment.
This LGTM once:
- We have a happy DCA run, and
- We've addressed the two
Omittable 'exists' variablealerts. I think they highlight the possibility of a nice cleanup.
d991181 to
c9c93ca
Compare
…in the dataflow stages of the query.
61b07b0 to
e32f7d8
Compare
|
Done. See analysis below. The TLDR is: Samate is very happy with these (on SamateAll the new results are classified as TPs in Samate 🎉. PHPLots of new results (the query had 0 results on this project before), and now has 1952 results. The majority of results are in an unrolled implementation of char trypath[MAXPATHLEN];
/* ... */
if (filename_length > (MAXPATHLEN - 2) || len > MAXPATHLEN || len + 1 + filename_length + 1 >= MAXPATHLEN) {
break;
}
memcpy(trypath, ptr, len);
trypath[len] = '/';ImageMagickAs PHP, we didn't have any results on this project before, but now we have ~3k results. The ones I've checked so far as been FPs like: status=FunctionImage(image,PolynomialFunction,2,coefficients,exception);(notice the MagickExport MagickBooleanType FunctionImage(Image *image,
const MagickFunction function,const size_t number_parameters,
const double *parameters,ExceptionInfo *exception)where we flow to q[i]=ApplyFunction(q[i],function,number_parameters,parameters, exception);which calls static Quantum ApplyFunction(Quantum pixel,const MagickFunction function,
const size_t number_parameters,const double *parameters,
ExceptionInfo *exception)which has code that looks like: width=(number_parameters >= 1) ? parameters[0] : 1.0;
center=(number_parameters >= 2) ? parameters[1] : 0.5;
range=(number_parameters >= 3) ? parameters[2] : 1.0; // <--- We raise an alert here.
bias=(number_parameters >= 4) ? parameters[3] : 0.5;But this is infeasible because WiresharkWe went from 10 to 450 results. Most of these new results actually looks like TPs to me. For example, this snippet looks kinda suspicious: if( length < sizeof name + 1 )
{
name[ length++ ] = (gchar) c;
}irregardless of what kind of array Similarly, this alert also looks super suspicious: static guint cid_adjust[MAX_CID];
/* ... */
for (i = 0; i < MAX_CID; i++) {
/* ... */
}
/* ... */
cid_index = i;
/* ... */
cid_adjust[cid_index] += cid_vernier[cid_index]; // <-- alert here.OpenJDKLots of new results. Before we had 10 and now we have 290. There are some FPs related to (once again) us not realizing that a path that crosses a function boundary is infeasible. For example here: double y[2];
/* ... */
n = __kernel_rem_pio2(tx,y,e0,nx,2,two_over_pi);which flows to: static int __kernel_rem_pio2(double *x, double *y, int e0, int nx, int prec, const int *ipio2) {which flows to: switch(prec) {
case 0:
...
break;
case 1:
case 2:
...
break;
case 3:
...
y[2] = fw; // <-- alert here
}so detecting that this is safe would just involve a call context of depth 1 (which the dataflow library already supports). But we haven't implemented the required predicate for C/C++: https://github.com/github/codeql/blob/main/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll#L812 VimWe go from ~70 results to ~380 results. The majority of them are code like this alert that start here: typval_T argv[2];
/* ... */
if (eval_expr_typval(expr, argv, 1, &rettv) == FAIL)which flows to: int
eval_expr_typval(typval_T *expr, typval_T *argv, int argc, typval_T *rettv)
{which passes down int
call_internal_method(
char_u *name,
int argcount,
typval_T *argvars,
typval_T *rettv,
typval_T *basetv)
{which finally flows to for (i = 2; i < argcount; ++i)
argv[i + 1] = argvars[i];
}
else if (global_functions[fi].f_argtype == FEARG_4)
{
// base value goes fourth
argv[0] = argvars[0];
argv[1] = argvars[1];
argv[2] = argvars[2]; // <-- alert here.but that's infeasible because |
|
I'm a little surprised e32f7d8 is a performance win, but DCA is definitely happier afterwards. LGTM |
Depends on #13045. Only
a96cc6dis new.